-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
B7161682765: Remove event as a function callback #154
B7161682765: Remove event as a function callback #154
Conversation
d2a4297
to
68751ed
Compare
e252b36
to
b26acc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 1 of 2 files at r3.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @josquindebaz and @ValentinaVasile)
src/event.js
line 140 at r2 (raw file):
* @param binding {eventBinding} Binding */ that.off = function (binding) {
this removal doesn't seem related to the current commit
test/events.test.js
line 154 at r1 (raw file):
}); it("Event Category keeps a list of events", () => {
these 2 tests are unrelated to this commit and still makes sense at this point I think. I would just replace the last 2 lines with
expect(anEvent.register).toBeTruthy();
expect(anotherEvent.register).toBeTruthy();
If you want to make the Git history completely atomic, this change in the test could go in a dedicated commit "Make some tests stop using deprecated code" or similar
test/events.test.js
line 92 at r2 (raw file):
}); it("Un-Bind callback using unregister", () => {
this removal doesn't seem related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @DamienCassou and @ValentinaVasile)
a discussion (no related file):
revised the commits to be less messy
src/event.js
line 140 at r2 (raw file):
Previously, DamienCassou (Damien Cassou) wrote…
this removal doesn't seem related to the current commit
ok
test/events.test.js
line 154 at r1 (raw file):
Previously, DamienCassou (Damien Cassou) wrote…
these 2 tests are unrelated to this commit and still makes sense at this point I think. I would just replace the last 2 lines with
expect(anEvent.register).toBeTruthy(); expect(anotherEvent.register).toBeTruthy();If you want to make the Git history completely atomic, this change in the test could go in a dedicated commit "Make some tests stop using deprecated code" or similar
thanks
test/events.test.js
line 92 at r2 (raw file):
Previously, DamienCassou (Damien Cassou) wrote…
this removal doesn't seem related
ok
b26acc0
to
7ca546f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r7, 2 of 2 files at r8, 2 of 2 files at r9, 2 of 2 files at r10, 2 of 2 files at r11, 2 of 2 files at r12, 2 of 2 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ValentinaVasile)
https://3.basecamp.com/4201305/buckets/11571123/todos/7161682765
This change is